-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Entire code base PR #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kosgrz, I just finished the code review and only have a couple of suggestions. These are non-blocking and could be converted to issues in the repo.
First off, thanks for all of this hard work!!
I am going to dive in and test the plugin now and verify all the features work as advertised! Awesome job!
Items discovered during plugin testing with dylan:
Bugs
|
Hi @kosgrz, I have done a round of testing with Bitbucket cloud. Here is a list of issues and questions I have so far. Testing with Jira integration and on server instance of BitBucket is ongoing. Sorry the list is long as this is quite a large undertaking as the plugin has alot of functionality. I have tried to organize the issues loosely by priority.
Other questions... @aaronrothschild maybe these features could wait for a later release? |
@kosgrz one quick follow-up on this. After integrating a Jira instance into my Bitbucket repo, I still see the same errors when trying to use the create and attach functionality. I have tried disconnecting and recxconnecting my user but, this did not change the behavior.
Please let me know if you have any thoughts? |
Thanks @DHaussermann for the review!
We should lock this capability down to ensure it is being observed.
It's important to get this working so the user can trust the system's messages to be accurate.
Yes, we should fix these before release. I'm 1/5, but this can be cleaned up in a future revision.
We need to fix this before shipping. @DHaussermann For #6-10 I created issues in this repo. @kosgrz sorry, I created them before I realized I couldn't label them with a "release" version. some of these are more important than others. We wanted to have the first release of this plugin by the end of this month hopefully. |
1 similar comment
Thanks @DHaussermann for the review!
We should lock this capability down to ensure it is being observed.
It's important to get this working so the user can trust the system's messages to be accurate.
Yes, we should fix these before release. I'm 1/5, but this can be cleaned up in a future revision.
We need to fix this before shipping. @DHaussermann For #6-10 I created issues in this repo. @kosgrz sorry, I created them before I realized I couldn't label them with a "release" version. some of these are more important than others. We wanted to have the first release of this plugin by the end of this month hopefully. |
Hi, thank you for the review and testing! @aaronrothschild, I will start fixing these bugs from today. I think I will finish it before the end of the week 👍 . |
@DHaussermann, it's strange, because it works on my instance. Could you send me logs with this error? It will help me with debugging. |
@kosgrz I have started another round of testing with your latest commit. I notice issues 8. and 9. Seem top be resolved but I'm not sure what else has changed. Regarding your question on 5. When I have no issues assigned to me in the paired jira instance (I do have PR to review) I see To take a bit of a tangent here... How should this work exactly? My connected user has a cc @aaronrothschild Just a heads up as 1. did not show up on your must fix list. For me the functionality where BitBucket is aware of a corresponding Jira project does not seem to be working. |
@kosgrz! Thanks for the prompt PR changes! @aaronrothschild, I'm approving, from a code review standpoint. We need a Security Review and the final QA review and we have a great bitbucket plugin |
@DHaussermann, thank you for the logs. It was very helpful. Command |
Thank you for the last commit @kosgrz I will do another round of testing as soon as I get a chance. The strange thing is. My Bitbucket repo does have a Jira instance attached so, I'm not sure why the plugin is not aware of this or if there is a config change I can make to resolve it. On a similar not. The post menu options should not be available if the pluging does not see a Jira instance associated to the Repo. Any thoughts on what's happening here? |
Hi @kosgrz I have done a round of testing on the last commit. Just to add clarity here - I have re-tested all the issues and summarized below. 1. Not able to use create and attach options even when I have a Jira instance connected |
@DHaussermann, thanks for testing! I have some questions below:
Unfortunately, I could not reproduce that. I had errors, only when I wanted to create an issue in a repository that does not have a Jira instance connected. What happens exactly when you try to create an issue?
It is true that this flag does not work. Sorry, I somehow missed that feature. I can add it quickly, but I would need to disable the option to subscribe to a whole organization when the flag is enabled. If a user wants to subscribe to a specific repository, I can check whether they have access there. But, if they want to subscribe to a whole organization, I cannot. Would this solution be ok for now? Are points 4 and 5 needed to be fixed before release?
Fixed 👍 . |
Confirmed 5. is fixed. Todo and issues icon on Team sidebar both work as as expected. No breaking CSS 👍 For issue 1., we may need a couple changes. One to resolve whatever issue I'm seeing and another to remove the post menu options where there is no Jira instance associated or the connected Bitbucket user does not have access. Here are some details of what I see happen...
I double checked the mapped user and when visiting Bitbucket with the same user I have access to the issue. Tried to disconnect and reconnect to see if anything would change. I also tried with the 2nd user on my Bitbucket. When I connect with the plugin, I see that the repos change (and repos without issues are listed which seems like an issue). But when I use create, I see the same problem but with the other Mattermost username.
To my understand 5. is resolved but, 4. is still needed. @aaronrothschild may also have some thoughts on this. |
@DHaussermann, thank you for explaining the issue in detail. It was very helpful 👍.
Unfortunately, according to my knowledge, this is not possible with the current Mattermost Plugin API. I added some extra logging when creating an issue. Now, in the logs you should see something like this:
Fixed. Now you should see only repos with issue tracker enabled 👍.
Ok, I will try to push a fix for 5. tomorrow 👍. |
@kosgrz thank you for the logging. Interestingly, when I deployed this build now, the behavior changed which may shed some light on the issue. In the modal, I no longer see the repos listed to attempt ticket creation. When the user opens the create modal everything looks fine expect that it behaves as though there are simply no repos to return see screen. So then I looked at what happens earlier when the user connects....
Strangely on an earlier build I was seeing the repos in my modal. Can you take a look and see why this may be occurring? |
Thank you for notifying me about this issue. I fixed it in the latest commit. You should now see error information when fetching repos failed.
I think you just reached the limit of requests to the Bitbucket API. You can send 1,000 requests per hour: https://support.atlassian.com/bitbucket-cloud/docs/api-request-limits/. I've tried to fix the issue:
but there is no simple way to do it. The
|
Hi @kosgrz Thanks for the last commit. Sorry for the late reply. I have some time to focus on this in the next few days and would like to see it wrapped up :) So, Just to outline where we are.... On 1. One of the error is resolved but, I still see ...
@kosgrz, I'm not sure hitting the request limit is the issue since it's quite high and resets daily. So, you would think I sometimes see my repo? Can you clarify a couple things please: @aaronrothschild 0/5 if we can't get this part tested soon, how would you feel about removing issue and comment creation for the initial release in the interest of getting this wrapped up? On 2. This was already answered above...
@aaronrothschild We have 2 options here. 0/5 can we just remove the flag from the UI and document that this release will not prevent user from making subscriptions based on Public / Private repo? The alternative provided seems quite limiting. On 3. This seems to have been reolves by one of the last couple commits. I have all features tunred on and did some testing. I never see confusing subscription data about branches being deleted and recreated. 👍 On 4. Yes, I've done some testing and you're correct! it only shows a max of 5 commit in the branch creation payload. I misunderstood and thought it would just keep adding the entire history. That being said - This is far less serious an issue. We should address this if needed after we ship it. 👍 |
Hi @DHaussermann,
Yes, this is the reason.
It's not happening on my machine. To how many repos you do have access to on your test account in Bitbucket? So, you cannot see your repos in the modal at all? Even when you use the plugin for the first time after a few days? It sounds very strange :/ |
Hi @kosgrz,
Strangely, today I don't see any error when connecting the user but, I still see the same behavior where there is no repo returned in the list of reps. Seems you need to be able to see the issue before you can help further with this :) |
@DHaussermann, sure. You can invite me. It will be much easier to investigate the issue :) |
@kosgrz sure, I would need to do it via email. Seems I can't get an email for you by looking at your GitHub handle. Can you please share an email address? You could DM it to me on the Mattermost Community server maybe? |
Hey @kosgrz Propose:
Yes, let's removing issue and comment creation to get this out to beta while we try to fix this.
Agreed, Propose we cut this functionality. |
@aaronrothschild, Good news for the issue creation and comment creation. I was confused on what type of Issue tracker this works with on the Bitbucket side. This does seem to be working as expected when using the out-of-the-box issue tracker. @kosgrz helped me sort this out. So we only only be missing a change that removes the flag for Provate repos (as it is non-functional) |
@kosgrz I have done some testing with your latest commit.
Though I don't see it anymore when connecting the user. I see it a few seconds after. I'm not sure if there are any functional implications. Not sure if this could be related... while testing this I did notice something else, even when I have 2 repositories with the non-jira BitBucket issue tracker enabled, I only see one repo. Is this something you can repro? I added you to a 2nd repo in my workspace called Lately, we still have the issue that the post menu items will show even when if there is no issue tracker enabled in the available repos. This is not a solvable issue correct? |
@kosgrz @DHaussermann could we do something similar to jira, where we allow admins to disable the functionality of Attach/Create from the dropdown? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kosgrz
Following additional security issues have been identified while reviewing the plugin. Can you please fix the same. Please let me know if you need any additional information.
1) The Bitbucket refresh tokens are not encrypted and stored in DB
Severity: High
Steps:
- When a user connects to a bitbucket account, the connection refresh_token is stored as plain text in DB. This value is not encrypted and stored.
- As a result, anyone can obtain a new access_token and the point of having an encrypted access_token in DB will be lost.
curl -X POST -u "key:secret" https://bitbucket.org/site/oauth2/access_token -d grant_type=refresh_token -d refresh_token=refresh_token
- Since a Bitbucket comprises of a user's personal account also, anyone who possesses this info from DB, can access user's other private repositories not supposed to be shared.
2) Bitbucket webhook secret isn't used and hence the incoming bitbucket webhook is vulnerable and data can be tampered.
Severity: Moderate
Steps:
- Currently the incoming webhook URL is static and looks like https://<INSTANCE_URL>/plugins/bitbucket/webhook
- Since the URL is static, anyone can send a POST request to this URL mimicking the Bitbucket and post contents with phishing URL etc
Suggestions: Make the incoming webhook URL dynamic and allow user to regenerate this URL, for example:
https://<INSTANCE_URL>/plugins/bitbucket/webhook/<SOME_RANDOM_UNIQUE_UUID>
3) A note should be added on the Plugin configuration page and inform Administrators to whitelist valid IP addresses of Bitbucket:
https://support.atlassian.com/bitbucket-cloud/docs/manage-webhooks/
https://support.atlassian.com/bitbucket-cloud/docs/what-are-the-bitbucket-cloud-ip-addresses-i-should-use-to-configure-my-corporate-firewall/
Severity: Low
4) When the Bitbucket plugin is removed from System Console, all the previous values like OAuth Client, Secret, Encryption Key etc should also be removed
Severity: Moderate
Steps:
- Login as a sysadmin user and install the bitbucket plugin
- Visit the bitbucket configuration page and enter all the details and save.
- Now remove the bitbucket plugin.
- Install the plugin again and visit the bitbucket configuration page.
- Notice that the values are still being displayed even after being removed.
5) The Bitbucket OAuth Client Secret is displayed as plain text in the System Console. When the config is saved and page is reloaded, this should not be exposed. We should mask it.
Severity: Low
Steps:
- Login as a sysadmin user and install the bitbucket plugin.
- Visit the bitbucket configuration page and enter Oauth client ID and secret and save.
- Reload the page and notice that the secret is still displayed in plain text.
- Like other config pages, i.e say OAuth config page on System console, the secret key should be truncated and displayed as ******. It should not be returned as plain text in config API
6) Regular User is allowed to view and manage list of subscriptions belonging to a different user.
Severity: Low
Steps:
- Login as User1 on Mattermost.
- Connect to Bitbucket as User1.
- Subscribe to few private repositories using the command
/bitbucket subscribe user1/repo1
- On another browser, login as User2 with low priviliges and visit the same channel.
- Check subscriptions
/bitbucket subscribe list
and notice that it still displays repo1 which is a private repo of user1. - Unsubscribe using the command
/bitbucket unsubscribe user1/repo1
and notice that user is allowed to change the subscription belonging to a different user.
Expected: Subscriptions should be user based. Only the owner of the subscription should be allowed to view or unsubscribe.
|
@hanzei regarding issue 1, yes currently the key is also stored in the DB. We may have to find a better way to manage this and store it in a different place other than DB (also applicable for all other plugins which behave the same way) Regarding issue 4, okay got it. In that case, we can ignore this for now. Will have a separate discussion on how can this be addressed in future. We may have to display a message in UI informing the same to sysadmin when they remove a plugin. |
* cleanup some webhooks
@DHaussermann, @srkgupta, I've created tickets for all remaining Security items and would like to merge this PR and address the issues in individual PRs. I'll be working on the fixes |
Note that I rebased this PR to merge onto master. |
Merging and will address the remaining issues in individual PRs |
/check-cla |
No description provided.